-
Notifications
You must be signed in to change notification settings - Fork 133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unified search #600
Unified search #600
Conversation
Maybe 40 nextcloud/server#22120 (comment) ? |
I think the app icon looks good. Can we color-match it to the user's theme or is that a bad idea? If anything, just make it a dark gray because the dark black is a little jarring. |
Well black is better as that inverts to white for dark-theme then. Also it's consistent with all other apps. |
I did not think about inverting for dark mode, that is a great point. Matching the user theme is probably not a good idea. Looking through my app icons in the search results, I see .wav, .als, .html, .mp3, .vcf files are all a gray color. Maybe I'm missing something here. The gray can invert to a darker white as well, no? It's just easier on the eyes and looks much more clean. A stark black looks "outdated" if you know what I mean. |
20a7117
to
df9cfe9
Compare
59eb6e6
to
3d29e99
Compare
Thanks for you feedback, also in the Talk channel. I've implemented your suggestions and updated the screenshot. Time for a final review 😃 |
} elseif (strpos($route, Application::APP_ID . '.') === 0) { | ||
return -1; | ||
} | ||
return 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default is far too high.
Do not inject yourself above default nextcloud apps please
|
||
public function getOrder(string $route, array $routeParameters): int { | ||
if (strpos($route, 'files' . '.') === 0) { | ||
return 25; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you put yourself lower when you're on files?
if (strpos($route, 'files' . '.') === 0) { | ||
return 25; | ||
} elseif (strpos($route, Application::APP_ID . '.') === 0) { | ||
return -1; | ||
} | ||
return 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (strpos($route, 'files' . '.') === 0) { | |
return 25; | |
} elseif (strpos($route, Application::APP_ID . '.') === 0) { | |
return -1; | |
} | |
return 4; | |
if (strpos($route, Application::APP_ID . '.') === 0) { | |
return -1; | |
} | |
return 40; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So yeah, as korel already mentioned. Notes are files, but the notes app can present them better. So it should be the prefered app when you search anywhere over files, hence the 4
in general, because it should be before files which has 5. Then again on files you might prefer the files search (or not) which is why 25 was picked there, and -1 when in notes.
@skjnldsv This was attuned via https://cloud.nextcloud.com/call/b8rjbqp6 : Joas Schilling @nickvergessen 28.08.2020 08:08:
Jan C Borchardt @jancborchardt 09.09.2020 14:50:
I asked if this is fine for everybody and nobody disagreed. But we can restart the discussion again, if you want 😐. The start was this:
What's your suggestion to deal with this problem? @skjnldsv |
@korelstar thank you for the changes to the icons... it looks really great! For the result index... I definitely agree that our results need to be above the "Files" results. I think that putting a higher priority index is the best short term solution for that. This might be a more long term strategy (if it's not already implemented) but perhaps there should be a way to set one's application as the default for files based on type or location. So "All txt files in this folder should be listed to open with Notes"... so more granular than just overall indexes, because I think that's a recipe for confusion/disaster (as seen here, there are common cases where very "high" priority nextcloud apps need to be superceded by user apps... and I understand @skjnldsv hesitance because of this). Obviously this would have to be implemented in nextcloud search itself, and would take a while to plan/implement if it ever is even considered, so we have to have a stopgap in the meantime. |
Ah btw @korelstar same as for the Dashboard widget #614 (comment) – it would be cool if the subline would be the beginning of the note content instead of the category. :) |
3d29e99
to
0278027
Compare
I implemented the last improvements (e.g. show note's excerpt instead of category @jancborchardt ) and I will merge this now, due to the pending major release. @skjnldsv Sorry that I can't wait for your answer, but we let's try it with the current implementation and fix it afterwards, if there is a problem with it. |
You can still filter notes by the unified search input, server emits an event when the search string is updated, see e.g. https://github.com/nextcloud/tasks/blob/master/src/main.js#L90-L96 |
@raimund-schluessler Thanks for the hint, I will have a look into that. But client-side filtering didn't work anymore for some time, since we do not load the mote's content for all notes anymore. Hence, searching/filtering must be realized on behalf of the server. Therefore, I think the current approach is fine, at least for now. |
Screenshot
Open Questions
notes
provider and by thefiles
provider. However, thefiles
provider will open a note using the Text app and not using the Notes app. Therefore, it would be advantageous if notes will be shown first and files below. But I doubt that this is not desired.@nextcloud/notes @nextcloud/designers